-
Notifications
You must be signed in to change notification settings - Fork 33
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WD-11691] chore: minor UI change for vertical navigation resizing. #797
[WD-11691] chore: minor UI change for vertical navigation resizing. #797
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't introduce a new concept for scrolling. We currently have this function to set the lower part of the nav to scroll. It works by measuring its height and comparing it to the total available height. This way we get a smooth transition on resize of the browser. It should also work by enabling scrolling on collapse/un-collapse of the accordions in the navigation (like for storage) in all possible height/width combinations.
So either remove that function and find a pure css solution or remove the css solution and rely on the existing one.
ea04681
to
267317f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
267317f
to
e46492b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the demo server, the navigation doesn't scroll any more. I.e. when I shrink the browser windows height the lower part of the nav stays visible as required, but the middle part is cur with no way of reaching the hidden nav items. Is the demo not updated, or is it a problem with the changes?
That is interesting... On my local computer and when I run the demo server, I am getting the correct behaviour. @mas-who could you please confirm what you see? @edlerd , this is the main thing that I had issue with fixing earlier, but I have corrected this and done extensive testing since then. (As well as cleared my cache lol) |
e46492b
to
c4a3be5
Compare
That being said, I have found that the top container is not scrollable. I added a fixing line for this in my last commit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
QA looks good now, the linter has some errors, see below.
Signed-off-by: Nkeiruka <[email protected]>
c4a3be5
to
84a568d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, nice work 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Done
QA
Screenshots